`get_by_name()` to return `Result<Branch, OxenError>` instead of `Result<Option<Branch>, OxenError>` by subygan · Pull Request #407 · Oxen-AI/Oxen

subygan · 2026-04-01T14:51:59Z

There was a lot of boiler plate that we were handling None to return OxenError::repo_not_found() this PR simplifies that.

Important change is this:
https://github.com/Oxen-AI/Oxen/pull/407/changes#diff-36d674fb2bcad4130c6b6fff3f561bbcbfe2a28ed81a632f375c2ae7b9dbf442R26

…`Result<Option<Branch>, OxenError>`

coderabbitai · 2026-04-01T14:52:11Z

📝 Walkthrough

Walkthrough

Refactored branch lookup semantics: repositories::branches::get_by_name now returns Result<Branch, OxenError> (missing branch → explicit error). Call sites across core, repositories, server, and bindings were updated to either use the new direct lookup or preserve optional behavior via non-propagating calls; many tests adjusted accordingly.

Changes

Cohort / File(s)	Summary
Branches API `crates/lib/src/repositories/branches.rs`	Changed `get_by_name` signature to return `Result<Branch, OxenError>` (missing → `local_branch_not_found`); updated `get_by_name_or_current`, `exists`, and related helpers to follow new semantics.
Core v_latest `crates/lib/src/core/v_latest/branches.rs`, `crates/lib/src/core/v_latest/commits.rs`, `crates/lib/src/core/v_latest/merge.rs`, `crates/lib/src/core/v_latest/push.rs`, `crates/lib/src/core/v_latest/workspaces/commit.rs`	Reworked branch lookups to use the new `get_by_name` behavior or preserve optional handling; removed many manual `local_branch_not_found` mappings and adjusted control flow accordingly.
Repository layer `crates/lib/src/repositories/checkout.rs`, `crates/lib/src/repositories/entries.rs`, `crates/lib/src/repositories/load.rs`, `crates/lib/src/repositories/merge.rs` (tests), `crates/lib/src/repositories/revisions.rs`	Updated consumers to either propagate errors from `get_by_name` or call non-propagating variants; tests updated to remove `.unwrap()` on lookups.
API / Client & Python binding `crates/lib/src/api/client/repositories.rs`, `crates/oxen-py/src/py_repo.rs`	Tests and client binding removed `.unwrap()` on branch lookups; branch-missing behavior now follows API change (explicit error or optional path depending on call).
Server controllers `crates/server/src/controllers/branches.rs`, `crates/server/src/controllers/workspaces.rs`, `crates/server/src/controllers/workspaces/data_frames.rs`, `crates/server/src/params.rs`	Controller endpoints updated to map `get_by_name` errors to appropriate HTTP/not-found behavior; creation and existence checks now match new error semantics and distinguish BranchNotFound vs other errors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Oxen-AI/Oxen#482: Similar changes to how repositories::branches::get_by_name is called and how missing branches are handled.
Oxen-AI/Oxen#596: Overlapping edits to checkout control flow and branch resolution logic.
Oxen-AI/Oxen#575: Related modifications in workspace commit behavior where branch lookup semantics were adjusted.

Suggested reviewers

jcelliott
gschoeni

Poem

🐇 I hopped through refs and names today,
Unwrapped no more, I cleared the way.
Branches found or gently missed,
Errors now wear a clearer list—
Hooray for tidy hops and play! 🥕✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: updating get_by_name() to return Result<Branch, OxenError> instead of Result<Option, OxenError>.
Docstring Coverage	✅ Passed	Docstring coverage is 96.30% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description accurately reflects the changeset: it modifies `get_by_name()` return type from `Result<Option<Branch>, OxenError>` to `Result<Branch, OxenError>` to eliminate boilerplate None-handling across the codebase, which matches the extensive changes documented in the raw summary.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch get_by_name

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

CleanCut

get_by_name is a good improvement!

The old form (and the current get_by_name_maybe) provides no value. Anything that uses get_by_name_maybe should either change to use get_by_name to be simpler, or use with_ref_manager directly otherwise.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/lib/src/core/v_latest/commits.rs`:
- Around line 132-135: In get_commit_by_branch, don't convert Result errors to
None via .ok().flatten(); instead preserve and propagate errors from
repositories::branches::get_by_name_maybe and get_by_id. Replace the chained
.ok().flatten() calls with explicit error handling: call
repositories::branches::get_by_name_maybe(repo, branch_name)? to get a
Option<branch>, return Ok(None) if no branch, and if Some(branch) call
get_by_id(repo, &branch.commit_id)? and wrap that result in Ok(Some(commit));
this keeps errors from get_by_name_maybe and get_by_id (and the function
signature of get_commit_by_branch) intact.

In `@crates/server/src/controllers/branches.rs`:
- Around line 183-184: The code currently converts a missing from_name into a
generic OxenHttpError::NotFound; instead preserve the branch-specific 404 by
mapping None to remote_branch_not_found(repo, &data.from_name). Replace the
.ok_or(OxenHttpError::NotFound)? on the
repositories::branches::get_by_name_maybe(...) result with
.ok_or(remote_branch_not_found(repo, &data.from_name))? (or ok_or_else with that
call) so the handler returns the same remote_branch_not_found(...) error payload
used by other branch endpoints.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 867baebf-cfc3-4ae3-81cd-b603a4db5f6f

📥 Commits

Reviewing files that changed from the base of the PR and between 60389ea and c13788b.

📒 Files selected for processing (17)

crates/lib/src/api/client/repositories.rs
crates/lib/src/core/v_latest/branches.rs
crates/lib/src/core/v_latest/commits.rs
crates/lib/src/core/v_latest/merge.rs
crates/lib/src/core/v_latest/push.rs
crates/lib/src/core/v_latest/workspaces/commit.rs
crates/lib/src/repositories/branches.rs
crates/lib/src/repositories/checkout.rs
crates/lib/src/repositories/entries.rs
crates/lib/src/repositories/load.rs
crates/lib/src/repositories/merge.rs
crates/lib/src/repositories/revisions.rs
crates/oxen-py/src/py_repo.rs
crates/server/src/controllers/branches.rs
crates/server/src/controllers/workspaces.rs
crates/server/src/controllers/workspaces/data_frames.rs
crates/server/src/params.rs

💤 Files with no reviewable changes (1)

crates/lib/src/repositories/revisions.rs

coderabbitai · 2026-04-01T15:23:16Z

crates/lib/src/core/v_latest/commits.rs

+    repositories::branches::get_by_name_maybe(repo, branch_name)
        .ok()
        .flatten()
        .and_then(|branch| get_by_id(repo, &branch.commit_id).ok().flatten())


⚠️ Potential issue | 🟠 Major

Do not swallow lookup/storage errors in get_commit_by_branch.

Using .ok().flatten() converts real errors into None, which can misreport backend/ref failures as “Commit not found”.

Proposed fix (preserve error context)

-fn get_commit_by_ref<S: AsRef<str> + Clone>( +fn get_commit_by_ref<S: AsRef<str> + Clone>( repo: &LocalRepository, ref_name: S, ) -> Result<Commit, OxenError> { - get_by_id(repo, ref_name.clone())? - .or_else(|| get_commit_by_branch(repo, ref_name.as_ref())) - .ok_or_else(|| OxenError::basic_str("Commit not found")) + if let Some(commit) = get_by_id(repo, ref_name.clone())? { + return Ok(commit); + } + if let Some(commit) = get_commit_by_branch(repo, ref_name.as_ref())? { + return Ok(commit); + } + Err(OxenError::basic_str("Commit not found")) } -fn get_commit_by_branch(repo: &LocalRepository, branch_name: &str) -> Option<Commit> { - repositories::branches::get_by_name_maybe(repo, branch_name) - .ok() - .flatten() - .and_then(|branch| get_by_id(repo, &branch.commit_id).ok().flatten()) +fn get_commit_by_branch( + repo: &LocalRepository, + branch_name: &str, +) -> Result<Option<Commit>, OxenError> { + let Some(branch) = repositories::branches::get_by_name_maybe(repo, branch_name)? else { + return Ok(None); + }; + get_by_id(repo, &branch.commit_id) }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@crates/lib/src/core/v_latest/commits.rs` around lines 132 - 135, In get_commit_by_branch, don't convert Result errors to None via .ok().flatten(); instead preserve and propagate errors from repositories::branches::get_by_name_maybe and get_by_id. Replace the chained .ok().flatten() calls with explicit error handling: call repositories::branches::get_by_name_maybe(repo, branch_name)? to get a Option<branch>, return Ok(None) if no branch, and if Some(branch) call get_by_id(repo, &branch.commit_id)? and wrap that result in Ok(Some(commit)); this keeps errors from get_by_name_maybe and get_by_id (and the function signature of get_commit_by_branch) intact.

coderabbitai · 2026-04-01T15:23:16Z

crates/server/src/controllers/branches.rs

+    let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?
        .ok_or(OxenHttpError::NotFound)?;


⚠️ Potential issue | 🟡 Minor

Preserve the branch-specific 404 here.

from_name now falls back to a generic OxenHttpError::NotFound, while the other branch endpoints in this controller return remote_branch_not_found(...) for the same missing-branch condition. That makes POST /branches emit a different error payload/message than the rest of the branch API.

Suggested fix

- let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)? - .ok_or(OxenHttpError::NotFound)?; + let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)? + .ok_or_else(|| OxenError::remote_branch_not_found(&data.from_name))?;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?

.ok_or(OxenHttpError::NotFound)?;

let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?

.ok_or_else(|| OxenError::remote_branch_not_found(&data.from_name))?;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@crates/server/src/controllers/branches.rs` around lines 183 - 184, The code currently converts a missing from_name into a generic OxenHttpError::NotFound; instead preserve the branch-specific 404 by mapping None to remote_branch_not_found(repo, &data.from_name). Replace the .ok_or(OxenHttpError::NotFound)? on the repositories::branches::get_by_name_maybe(...) result with .ok_or(remote_branch_not_found(repo, &data.from_name))? (or ok_or_else with that call) so the handler returns the same remote_branch_not_found(...) error payload used by other branch endpoints.

subygan · 2026-04-01T16:06:55Z

with_ref_manager is a little lower level access, which was I wanted to keep the get_by_name_maybe indirection to keep it clean.

I suggest just removing get_by_name_maybe and catch with where needed. wdyt?

    match repositories::branches::get_by_name(repo, name) {
        Ok(branch) => Ok(Some(branch)),
        Err(OxenError::BranchNotFound(_)) => Ok(None),
        Err(e) => Err(e),
    }

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/lib/src/repositories/branches.rs`:
- Around line 56-58: The current exists(repo, name) function swallows all errors
from get_by_name(repo, name).is_ok(), treating I/O/db failures the same as
BranchNotFound; fix by either (A) changing exists signature to return
Result<bool, OxenError> and propagate get_by_name(repo, name)? so callers (e.g.,
the checkout logic that calls exists) can handle real errors, or (B) keep the
bool signature but explicitly match the error from get_by_name and return false
only for the BranchNotFound variant while propagating or returning an error for
other cases (i.e., call get_by_name and match Err(BranchNotFound) => false,
Err(other) => propagate/log/return error). Update the caller in checkout to use
the new Result<bool, OxenError> or handle the propagated error accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 95907e74-a290-41fe-9ac0-31b7c4813595

📥 Commits

Reviewing files that changed from the base of the PR and between c13788b and a1492a2.

📒 Files selected for processing (10)

crates/lib/src/core/v_latest/commits.rs
crates/lib/src/core/v_latest/workspaces/commit.rs
crates/lib/src/repositories/branches.rs
crates/lib/src/repositories/checkout.rs
crates/lib/src/repositories/entries.rs
crates/lib/src/repositories/revisions.rs
crates/server/src/controllers/branches.rs
crates/server/src/controllers/workspaces.rs
crates/server/src/controllers/workspaces/data_frames.rs
crates/server/src/params.rs

💤 Files with no reviewable changes (1)

crates/lib/src/core/v_latest/commits.rs

🚧 Files skipped from review as they are similar to previous changes (6)

crates/lib/src/repositories/entries.rs
crates/lib/src/repositories/checkout.rs
crates/lib/src/core/v_latest/workspaces/commit.rs
crates/lib/src/repositories/revisions.rs
crates/server/src/controllers/workspaces/data_frames.rs
crates/server/src/controllers/branches.rs

coderabbitai · 2026-04-01T16:14:11Z

crates/lib/src/repositories/branches.rs

+pub fn exists(repo: &LocalRepository, name: &str) -> bool {
+    get_by_name(repo, name).is_ok()
 }


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash # Check usages of branches::exists to see if callers handle errors rg -n -C3 'branches::exists'

Repository: Oxen-AI/Oxen

Length of output: 730

🏁 Script executed:

# Find get_by_name implementation rg -A15 'fn get_by_name' crates/lib/src/repositories/branches.rs | head -40

Repository: Oxen-AI/Oxen

Length of output: 922

🏁 Script executed:

# Also check OxenError types to understand BranchNotFound rg -B2 -A2 'BranchNotFound' crates/lib/src/ --type rust | head -30

Repository: Oxen-AI/Oxen

Length of output: 1971

exists() silently treats lookup errors as "not found".

The implementation returns false for any error from get_by_name(), including I/O or database failures, not just BranchNotFound. While this matches the single usage pattern (as a simple boolean guard in checkout.rs:23), it does mask underlying errors.

However, the current design appears intentional: the API is a simple existence check, and callers that need the actual branch follow up with get_by_name()? for error handling. The suggested alternative of returning Result<bool, OxenError> would require updating the existing caller's usage pattern. If preserving error distinction is important, consider documenting the error-suppression behavior or slightly refactoring the checkout logic to propagate lookup errors more explicitly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@crates/lib/src/repositories/branches.rs` around lines 56 - 58, The current exists(repo, name) function swallows all errors from get_by_name(repo, name).is_ok(), treating I/O/db failures the same as BranchNotFound; fix by either (A) changing exists signature to return Result<bool, OxenError> and propagate get_by_name(repo, name)? so callers (e.g., the checkout logic that calls exists) can handle real errors, or (B) keep the bool signature but explicitly match the error from get_by_name and return false only for the BranchNotFound variant while propagating or returning an error for other cases (i.e., call get_by_name and match Err(BranchNotFound) => false, Err(other) => propagate/log/return error). Update the caller in checkout to use the new Result<bool, OxenError> or handle the propagated error accordingly.

malcolmgreaves · 2026-04-01T16:52:21Z

crates/lib/src/repositories/branches.rs

-        Some(_) => Ok(true),
-        None => Ok(false),
-    }
+pub fn exists(repo: &LocalRepository, name: &str) -> bool {


I could also see this being:

match get_by_name(repo,name) { Ok(_) => Ok(true), Err(OxenError::BranchNotFound(_)) => Ok(false), error => error, }

And keeping it as Result<bool, OxenError> and adding the following to the docstring:

/// Check if a branch exists. Returns Ok(false) if the branch doesn't exist. Returns Err() if there was some other problem accessing the local repository.

use get_by_name() to return Result<Branch, OxenError> instead of …

28c41ad

…`Result<Option<Branch>, OxenError>`

subygan marked this pull request as ready for review April 1, 2026 15:11

Merge branch 'main' into get_by_name

c13788b

CleanCut reviewed Apr 1, 2026

View reviewed changes

coderabbitai bot reviewed Apr 1, 2026

View reviewed changes

subygan added 2 commits April 1, 2026 12:08

remove get_by_name_maybe

d4033cf

Merge branch 'get_by_name' of github.com:Oxen-AI/Oxen into get_by_name

a1492a2

coderabbitai bot reviewed Apr 1, 2026

View reviewed changes

malcolmgreaves reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`get_by_name()` to return `Result<Branch, OxenError>` instead of `Result<Option<Branch>, OxenError>`#407

`get_by_name()` to return `Result<Branch, OxenError>` instead of `Result<Option<Branch>, OxenError>`#407
subygan wants to merge 4 commits intomainfrom
get_by_name

subygan commented Apr 1, 2026 •

edited by malcolmgreaves

Loading

Uh oh!

coderabbitai bot commented Apr 1, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

CleanCut left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Apr 1, 2026

Uh oh!

coderabbitai bot Apr 1, 2026

Uh oh!

subygan commented Apr 1, 2026 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Apr 1, 2026

Uh oh!

malcolmgreaves Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		let from_branch = repositories::branches::get_by_name_maybe(repo, &data.from_name)?
		.ok_or(OxenHttpError::NotFound)?;

Conversation

subygan commented Apr 1, 2026 • edited by malcolmgreaves Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

CleanCut left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

subygan commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

malcolmgreaves Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

subygan commented Apr 1, 2026 •

edited by malcolmgreaves

Loading

coderabbitai bot commented Apr 1, 2026 •

edited

Loading

subygan commented Apr 1, 2026 •

edited

Loading